Skip to content
This repository has been archived by the owner on Nov 10, 2017. It is now read-only.

Speed improvements #486

Closed
wants to merge 4 commits into from
Closed

Conversation

fitzgen
Copy link

@fitzgen fitzgen commented Apr 26, 2015

Before

screen shot 2015-04-26 at 3 56 06 pm

After

screen shot 2015-04-26 at 3 54 06 pm

That one last FPS dip is completely within angular. I suspect making it go away would involve significant changes within angular, or completely ditching angular in treeherder.

Review on Reviewable

@fitzgen
Copy link
Author

fitzgen commented Apr 26, 2015

Also, I couldn't for the life of me get the unit tests to run. This is what I get:

$ ./scripts/test.sh 

Starting Karma Server (http://karma-runner.github.io)
-------------------------------------------------------------------
INFO [karma]: Karma v0.12.31 server started at http://localhost:9876/
INFO [launcher]: Starting browser Firefox
INFO [Firefox 36.0.0 (Mac OS X 10.10)]: Connected on socket KFbF3sPG-CNXcyJF663y with id 84325436
Firefox 36.0.0 (Mac OS X 10.10) ERROR
  TypeError: jasmine.Matchers is undefined
  at /Users/fitzgen/src/treeherder-ui/webapp/test/vendor/jasmine-jquery.js:530

@fitzgen
Copy link
Author

fitzgen commented Apr 26, 2015

Hrm, seems the tests failed on travis-ci, so maybe these results are bunk (but everything seems to work fine locally for me).

If anyone has ideas on why the unit tests don't run locally for me, I'm all ears...

@tojon
Copy link

tojon commented Apr 27, 2015

@fitzgen those results look amazing. Is there an open bug to which I can assign this PR? We have a meta UI performance bug 1067846, and a depends bug 1125229 in case either of those are applicable.

I defer to @maurodoglio and @edmorley on the current CI failures.

@maurodoglio
Copy link

@fitzgen I tried your branch locally, it's a big improvement indeed 😃 , thank you!
I'll try to figure out why the travis run is failing.
Regarding the error you get locally, you may be using an outdated version of jasmine/jasmine-jquery. From a fresh node environment npm install && npm test is all you need to run the tests. We probably need to pin some more packages in package.json 😞

item_list = [];
var props = response.data.job_property_names;
var propsLen = props.length;
for (var i = 0, resultsLen = response.data.results.length; i < propsLen; i++) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition to break the loop should be i < resultsLen
I think this is the reason why the tests are failing. You probably don't see the problem if you look at mozilla-central, since the number of jobs per result set is > 36. On try you will likely see errors in the console.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before breaking this stuff out into loops (and making the code much less readable), I wonder if we should consider using lodash (https://lodash.com/) instead of underscore. It's supposed to be quite a bit faster than underscore (http://benmccormick.org/2014/11/12/underscore-vs-lodash/), and indeed uses the technique you're using here to iterate through arrays with map (and other things): https://github.com/lodash/lodash/blob/master/lodash.js#L1443

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The condition to break the loop should be i < resultsLen
I think this is the reason why the tests are failing.

Yes! Thanks! All passing now.

@edmorley
Copy link
Contributor

Awesome - thank you for digging into this!
For the jquery change - is there an upstream issue open for it? Would be awesome to get it fixed upstream if possible :-)

// creates a new instance of ThJobModel
// using the provided properties
angular.extend(this, data);
if (typeof data === "object" && data !== null) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need a comment here to explain why we're not using angular.extend, otherwise someone might be tempted to change it back (since the original form is more concise).

@wlach
Copy link

wlach commented Apr 27, 2015

So first off, thanks for doing this! I have some concerns about some of the implementation details, but the basic ideas seem good. Some questions:

  1. How were you interacting with treeherder when you were generating the above profiles?
  2. What relative speed increases did you get from each of the above commits?

As @tojonmz mentioned, we should open a bug to associate this work with.

@fitzgen
Copy link
Author

fitzgen commented Apr 27, 2015

How were you interacting with treeherder when you were generating the above profiles?

This is scrolling to the bottom, start profiling, clicking on "load 50 more", waiting for them all to load, end profiling.

What relative speed increases did you get from each of the above commits?

Mostly from ditching underscore to (a) avoid allocations and reduce GC pressure, and (b) get better interaction with the JIT. That's the big FPS gain you can see throughout the middle of the recording. The change to push instead of concat helps ease GC pressure here as well.

We could probably ditch the ThJobModel constructor change and instead have a factory function something like this:

ThJobModel.constructWithProperties = function (properties, values) {
    var model = new ThJobModel();
    ... add properties ...
    return model;
};

That would clean up the code organization a little bit so that the properties aren't added in the callback and things are a bit better encapsulated.

The jQuery fix causes that initial FPS dip to go from ~300ms down to ~150ms. I talked to @mzgol on twitter, not sure if he filed a bug.

npm install && npm test

Ok, that worked for me, thanks. FWIW, readthedocs suggests other stuff with -g and craziness.

@fitzgen
Copy link
Author

fitzgen commented Apr 27, 2015

Ok, so as I feared, the fact that the tests were failing and UI responsiveness improved were related :)

We were creating a ton less models because we were only making as many models as there are properties. With the bug fixed, these changes (other than the jQuery one) don't actually help too much. Back to the drawing board!

@mgol
Copy link

mgol commented May 20, 2015

@fitzgen do you happen to have a relatively small example that shows the performance penalty of copying event properties in Firefox? I'd like to dive in but I need something to start from and preferably something that is not extremely huge. :)

@fitzgen
Copy link
Author

fitzgen commented May 20, 2015

Sorry, I don't. I suspect it isn't a bottleneck unless there is a bunch of stuff requiring reflow, so a smaller test case may be hard to create.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants